Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

command: adjust exit code of state rm #22300

Merged
merged 8 commits into from
Jun 24, 2020
Merged

command: adjust exit code of state rm #22300

merged 8 commits into from
Jun 24, 2020

Conversation

jgpacker
Copy link
Contributor

@jgpacker jgpacker commented Aug 1, 2019 •
edited
Loading

Command state rm will now exit with code 1 when the resource is not found in the current state.
This is consistent with terraform state show non_existent_resource.

Fixes #17800

command: adjust exit code of state rm and state mv
1151800 
Commands `state rm` and `state mv` will now exit with code 1 when the
target resource is not found in the current state.
This is consistent with `terraform state show non_existent_resource`.

Fixes hashicorp#17800
@jgpacker jgpacker changed the title [WIP] command: adjust exit code of state rm and state mv command: adjust exit code of state rm and state mv Aug 2, 2019
@mildwonkey mildwonkey requested a review from a team August 2, 2019 12:25
pselle
pselle previously requested changes Aug 5, 2019
View reviewed changes
Copy link
Contributor

@pselle pselle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jgpacker, thanks for this contribution! I tried out your patch and it works as described, however, now that exit code is 1, we should show an error to the user versus showing UI output -- so that the experience is consistent (the command errored, so an error is displayed).

In Terraform, our best practice to add an error is using the tfdiags package, where a summary, and then more helpful information is shared so the user can amend their error. In this scenario, something like (this uses some language from the error displayed when you use terraform show on the non-existent resource) could replace the current UI output in state rm:

		diags = diags.Append(tfdiags.Sourceless(
			tfdiags.Error,
			"Invalid target address",
			"No matching objects found. To view the available instances, use "terraform state list". Please modify the address to reference a specific instance.",
		))
		c.showDiagnostics(diags)
		return 1

@jgpacker
Copy link
Contributor Author

jgpacker commented Aug 5, 2019

Thank you for the feedback. I will correct this soon

@jgpacker
Copy link
Contributor Author

jgpacker commented Aug 24, 2019 •
edited
Loading

I have updated the code according to the feedback. Unfortunately I have not been able to test it locally right now. So there might be still be some adjustments to be done. Thank you in advance for the review

@pselle pselle self-requested a review August 26, 2019 15:51
pselle
pselle reviewed Sep 9, 2019
View reviewed changes
Copy link
Contributor

@pselle pselle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One improvement for state_mv, since we have a constant that already exists.

I had trouble figuring out a flow of terraform state mv that would hit this while testing on my machine -- perhaps you could add a test to state_mv_test.go hitting this error?

command/state_mv.go Outdated Show resolved Hide resolved
@pselle pselle dismissed their stale review September 9, 2019 20:35

Outdated, changes made

@jgpacker
Copy link
Contributor Author

@pselle I opened this PR only to fix state rm, but I wanted to make sure other state subcommands were consistent with this behavior. I changed state mv because there is an if related to this condition, but I'm not sure how to reproduce it in this case.
I would not be surprised if this condition is not reached at this moment.
Should we remove this condition? Make the error message more generic?
What do you suggest?

@pselle
Copy link
Contributor

pselle commented Sep 25, 2019

Ah in that case (if you haven't been able to replicate through with state mv and the goal was state rm with mv being a "while I'm here...") I would suggest we only fix state rm and leave mv be. That follows the line of keeping a change focused and tested, so while mv might have some tricky bits in it, if changing it is not our goal, we should leave it unchanged and leave that for future work :)

@jgpacker
Copy link
Contributor Author

Thank you for the orientation. I have reverted the changes in state_mv. Please review again

@jgpacker jgpacker requested a review from pselle October 27, 2019 21:35
pselle
pselle approved these changes Oct 28, 2019
View reviewed changes
Copy link
Contributor

@pselle pselle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the nudge @jgpacker. I'm approving this, but tagging it with "breaking-change" as the related issue has the same tag, so this will need to go out in the 0.13 release vs a 0.12.x release.

@danieldreier danieldreier added this to the v0.13.0 milestone Oct 30, 2019
@jgpacker jgpacker changed the title command: adjust exit code of state rm and state mv command: adjust exit code of state rm Dec 12, 2019
@pselle pselle merged commit fd47260 into hashicorp:master Jun 24, 2020
@jgpacker jgpacker deleted the fix-state-rm-n-mv-exit-code branch June 25, 2020 14:29
@ghost
Copy link

ghost commented Jul 25, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Jul 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pselle pselle pselle approved these changes

Assignees
No one assigned
Labels
breaking-change bug cli
Projects
None yet
Milestone
v0.13.0
Development

Successfully merging this pull request may close these issues.

terraform state rm always displays success message as long as you give it a "valid" resource name
4 participants
@jgpacker @pselle @danieldreier @teamterraform